Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove go.mod replace for client-go by extracting required parts of boskos #26956

Merged
merged 2 commits into from
Jul 29, 2022
Merged

Remove go.mod replace for client-go by extracting required parts of boskos #26956

merged 2 commits into from
Jul 29, 2022

Conversation

munnerz
Copy link
Member

@munnerz munnerz commented Jul 28, 2022 •
edited
Loading

Extracts the parts of boskos that this repository actually uses into a third_party directory.

This allows us to remove the replace directive, which will in turn allow for a PR against the boskos repository to do the same (after bumping to main of k8s.io/test-infra).

Once that's been done, I can create a PR to basically revert this PR and bump our dependency on boskos 😅

Closes #20421

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/boskos Issues or PRs related to code in /boskos area/config Issues or PRs related to code in /config area/kubetest area/prow/bump Updates to the k8s prow cluster sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jul 28, 2022
@munnerz munnerz mentioned this pull request Jul 28, 2022
Closed
@munnerz
Copy link
Member Author

munnerz commented Jul 28, 2022

/retest

@munnerz
Copy link
Member Author

munnerz commented Jul 28, 2022

/retest

seems like integration tests are failing on something that looks totally unrelated... 👀

@BenTheElder
Copy link
Member

let's put this package under kubetest/ and add a README about where the code came from and how other users should probably consume the upstream client library instead.

@munnerz
Copy link
Member Author

munnerz commented Jul 29, 2022

My plan was to remove these files altogether once the boskos repository has upgraded to not depend on v11 of client-go - happy to write up a README advising others, but I wasn't anticipating this being very permanent 😊

@munnerz munnerz force-pushed the fixup-client-go branch 2 times, most recently from d3b2dfe to 4db4241 Compare July 29, 2022 10:39
@munnerz
Copy link
Member Author

munnerz commented Jul 29, 2022

Moved into kubetest/boskos and added a README 😄

munnerz
munnerz commented Jul 29, 2022
View reviewed changes
kubetest/boskos/common/common.go Outdated
type ResourceEntry struct {
Type string `json:"type"`
State string `json:"state"`
Names []string `json:"names,flow"`
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm looking into the origins of this flow statement (which trips up golangci-lint) and it seems to have been there since the very origins of boskos/mason: https://github.com/kubernetes/test-infra/pull/6676/files

It's not a valid json string, and I believe we can remove it altogether.. I'm going to add a commit to delete it, but if anyone knows what it's for or why it's needed, please speak now 😄

alvaroaleman
alvaroaleman reviewed Jul 29, 2022
View reviewed changes
kubetest/boskos/common/common.go
@@ -116,7 +116,7 @@ type Resource struct {
type ResourceEntry struct {
Type string `json:"type"`
State string `json:"state"`
Names []string `json:"names,flow"`
Names []string `json:"names"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems we got really lucky with this one, the go json encoder still respects the first value and just ignores the flow

alvaroaleman
alvaroaleman approved these changes Jul 29, 2022
View reviewed changes
Copy link
Member

@alvaroaleman alvaroaleman left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, but can't approve root level changes

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 29, 2022
BenTheElder
BenTheElder approved these changes Jul 29, 2022
View reviewed changes
Copy link
Member

@BenTheElder BenTheElder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve
Thanks James!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, BenTheElder, munnerz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 29, 2022
@k8s-ci-robot k8s-ci-robot merged commit 07f065e into kubernetes:master Jul 29, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.25 milestone Jul 29, 2022
This was referenced Aug 1, 2022
Closed
Open
@BenTheElder BenTheElder mentioned this pull request Aug 2, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenTheElder BenTheElder BenTheElder approved these changes

@alvaroaleman alvaroaleman alvaroaleman approved these changes

@cblecker cblecker Awaiting requested review from cblecker

@feiskyer feiskyer Awaiting requested review from feiskyer

Assignees

@BenTheElder BenTheElder

@alvaroaleman alvaroaleman

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/boskos Issues or PRs related to code in /boskos area/config Issues or PRs related to code in /config area/kubetest area/prow/bump Updates to the k8s prow cluster cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.25
Development

Successfully merging this pull request may close these issues.

client-go and boskos imports lead to go.mod replace hacks
4 participants
@munnerz @BenTheElder @k8s-ci-robot @alvaroaleman